netty: disable huffman coding in headers #10563

fedorka · 2023-09-15T21:23:40Z

When operating gRPC clients which are not bandwidth constrained (eg: calling a collocated proxy), clients may want to spend less CPU on header encoding while allowing larger message sizes. This allows such clients to configure the HPACK huffman coding threshold to influence this tradeoff.

Anecdotally, when calling sidecars via gRPC, we have been setting the threshold to Integer.MAX_VALUE. This saves CPU on header encoding in the application and again on decoding on the sidecar.

This improves the header encoding performance noted in #1670.

Thanks to @drobertduke for performing the initial research.

linux-foundation-easycla · 2023-09-15T21:23:44Z

The committers listed above are authorized under a signed CLA.

✅ login: fedorka / name: Benjamin Fedorka (b66664a, faeda73, 3614be8, a2819f3, be1bbd5)

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java

fedorka · 2023-09-19T00:15:50Z

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java

+
+    new Rpc(transport, headers).halfClose().waitForResponse();
+
+    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.MILLISECONDS);


There appears to be a race between the RPC completing/releasing this thread and the traffic counter finishing the counting of the bytes. I'm working to remove this hack.

ejona86 · 2023-09-19T22:14:42Z

This seems to be different from #1670. #1670 wasn't concerned with Huffman. I agree that Huffman is generally not worth the cost. But I'm most surprised that 512 isn't a big enough threshold for you. What sort of headers are you transferring that is so large?

fedorka · 2023-09-20T00:04:31Z

I may have misunderstood #1670, I understood it as a general performance issues for time spent within encodeHeaders.

We use headers to propagate an extensive amount of request-specific metadata (eg: special routing, failure injection information, and authentication details). Some of this metadata is serialized to binary, then base64 encoded, and passed as a single header (instead of multiple smaller headers) in the 800-1200 character range.

A long standing pattern in our ecosystem is to inject these headers at a high level and propagate them through the RPC chain (instead of using an independent side-channel) so this impacts a broad range of our internal services.

ejona86 · 2023-09-28T23:14:47Z

I've spoken with some others, and they agree: huffman in the datacenter doesn't add much value in the common cases. And especially based on "the header is 512+ bytes." It could be useful to turn on huffman based on the connection latency (say, >10ms means "assume cross-datacenter") but the Netty API doesn't lend it to that. The savings here aren't huge and it is expensive; the table provides the biggest savings.

Let's just disable huffman unconditionally. The "Netty doesn't have an API for this in the new API" is a concern, but let's kick that can down the road. This is the least of our problems for swapping to the new API.

…ChannelBuilder, update tests to be predictable on coded size.

ejona86

I'm a bit wary of assuming RPCs will be exactly the same size each time they are sent. But there's no grpc-timeout, so... yeah, they will end up being identical size. Seems okay.

I'm going to be on vacation, but this approach seems fine.

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java

ejona86 · 2023-10-10T04:25:03Z

note to merger: this PR has changed purpose (no longer about configuration), so we'll need to make sure the messages make sense when we squash and merge.

larry-safran · 2023-10-10T19:35:51Z

netty/src/main/java/io/grpc/netty/NettyClientHandler.java

@@ -150,7 +152,9 @@ static NettyClientHandler newHandler(
    Preconditions.checkArgument(maxHeaderListSize > 0, "maxHeaderListSize must be positive");
    Http2HeadersDecoder headersDecoder = new GrpcHttp2ClientHeadersDecoder(maxHeaderListSize);
    Http2FrameReader frameReader = new DefaultHttp2FrameReader(headersDecoder);
-    Http2FrameWriter frameWriter = new DefaultHttp2FrameWriter();
+    Http2HeadersEncoder encoder = new DefaultHttp2HeadersEncoder(


How does this not get thrown away when you call newHandler below?

It is passed to the frameWriter on the next line.

fedorka · 2023-11-08T17:30:40Z

I finally got time to take another pass at this 🙂

The test is now directly asserting on the data in the header and no longer suffers from the previous stability issues.

Though there was some thrashing in the branch, the diff against e1334eae7bba39d85a952bc5ab5aeb4cb05a56d8 is clean so this should squash nicely.

ejona86

Ah, yeah, the test works reasonably just checking for the string among the bytes.

anuragagarwal561994 · 2023-11-19T12:46:00Z

@ejona86 any planned release date for this MR?

ejona86 · 2023-11-20T19:34:08Z

@anuragagarwal561994, this made it onto the 1.60.x branch. The 1.60 milestone is scheduled for next week.

anuragagarwal561994 · 2023-11-20T22:08:05Z

Thanks for update @ejona86 Black Friday is coming, I thought could get a boost before that 😄 but anyways, I have added some hack to get it working for our use-case.

netty: allow the HPACK Huffman coding threshold to be configured

b66664a

fedorka commented Sep 15, 2023

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java Show resolved Hide resolved

attempt to stabilize tests

faeda73

fedorka commented Sep 19, 2023

View reviewed changes

Always disable huffman coding, remove configuration option from Netty…

3614be8

…ChannelBuilder, update tests to be predictable on coded size.

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 10, 2023

grpc-kokoro removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Oct 10, 2023

ejona86 approved these changes Oct 10, 2023

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java Show resolved Hide resolved

ejona86 requested a review from larry-safran October 10, 2023 04:23

larry-safran reviewed Oct 10, 2023

View reviewed changes

fedorka added 2 commits November 8, 2023 12:19

Change huffman coding tests to directly assert on header contents

a2819f3

correct message in assertion

be1bbd5

fedorka changed the title ~~netty: allow the HPACK Huffman coding threshold to be configured~~ netty: disable huffman coding in headers Nov 8, 2023

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2023

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2023

ejona86 approved these changes Nov 13, 2023

View reviewed changes

ejona86 requested a review from YifeiZhuang November 13, 2023 17:55

YifeiZhuang approved these changes Nov 14, 2023

View reviewed changes

YifeiZhuang merged commit 2b65e66 into grpc:master Nov 14, 2023
13 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netty: disable huffman coding in headers #10563

netty: disable huffman coding in headers #10563

fedorka commented Sep 15, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 15, 2023 •

edited

Loading

fedorka Sep 19, 2023

ejona86 commented Sep 19, 2023

fedorka commented Sep 20, 2023

ejona86 commented Sep 28, 2023

ejona86 left a comment •

edited

Loading

ejona86 commented Oct 10, 2023

larry-safran Oct 10, 2023

ejona86 Nov 8, 2023

fedorka commented Nov 8, 2023

ejona86 left a comment

anuragagarwal561994 commented Nov 19, 2023

ejona86 commented Nov 20, 2023

anuragagarwal561994 commented Nov 20, 2023 •

edited

Loading


		new Rpc(transport, headers).halfClose().waitForResponse();

		Uninterruptibles.sleepUninterruptibly(1, TimeUnit.MILLISECONDS);

netty: disable huffman coding in headers #10563

netty: disable huffman coding in headers #10563

Conversation

fedorka commented Sep 15, 2023 • edited Loading

linux-foundation-easycla bot commented Sep 15, 2023 • edited Loading

fedorka Sep 19, 2023

Choose a reason for hiding this comment

ejona86 commented Sep 19, 2023

fedorka commented Sep 20, 2023

ejona86 commented Sep 28, 2023

ejona86 left a comment • edited Loading

Choose a reason for hiding this comment

ejona86 commented Oct 10, 2023

larry-safran Oct 10, 2023

Choose a reason for hiding this comment

ejona86 Nov 8, 2023

Choose a reason for hiding this comment

fedorka commented Nov 8, 2023

ejona86 left a comment

Choose a reason for hiding this comment

anuragagarwal561994 commented Nov 19, 2023

ejona86 commented Nov 20, 2023

anuragagarwal561994 commented Nov 20, 2023 • edited Loading

fedorka commented Sep 15, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 15, 2023 •

edited

Loading

ejona86 left a comment •

edited

Loading

anuragagarwal561994 commented Nov 20, 2023 •

edited

Loading